-
Notifications
You must be signed in to change notification settings - Fork 318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove experimental from MapboxNavigationApp #6143
Conversation
fe92387
to
c194f9b
Compare
Codecov Report
@@ Coverage Diff @@
## main #6143 +/- ##
============================================
- Coverage 68.90% 68.89% -0.01%
Complexity 4255 4255
============================================
Files 639 639
Lines 25646 25643 -3
Branches 3003 3003
============================================
- Hits 17671 17668 -3
Misses 6828 6828
Partials 1147 1147
|
c194f9b
to
fe6409f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!! Glad to see we are moving towards stability.
Is there a ticket to update all of our examples? |
libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt
Show resolved
Hide resolved
Should we somehow discourage users from instantiating |
🚢
A small problem I see with the |
libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigationProvider.kt
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigationProvider.kt
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigationProvider.kt
Show resolved
Hide resolved
Additionally to deprecating, maybe we should update all |
c46af8b
to
7b095a4
Compare
Thanks for comments! Responses are below the comment links.
Not yet, we need a
This topic has come up before, we are discouraging it. it would also be a SEMVER to prohibit it strictly. I think we are being careful to decide if the public constructor should deprecated. It does add support for a simple synchronous construction as lukas mentioned.
Adding to this. We are discouraging synchronous creation of mapboxNavigation = MapboxNavigationApp.setup(navigationOptions).attach(lifecycleOwner).current()!!
We need to support Any changes to |
7b095a4
to
5b0e366
Compare
5b0e366
to
0b6453c
Compare
@@ -840,8 +841,9 @@ class MapboxNavigation @VisibleForTesting internal constructor( | |||
* versionInfo: RoadGraphVersionInfo? | |||
* ) { | |||
* if (isUpdateAvailable) { | |||
* mapboxNavigation.onDestroy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This had some pretty bad consequences. Considering onDestroy will remove all registered listener
When using MapboxNavigationApp
, this recreation can be done and listeners can be re-registered in MapboxNavigationObserver.onAttached
We are not entirely ready to migrate all examples (the release is still in alpha). We could migrate the examples now, but there are many helpful extensions we should consider that will makes migration simpler. Here is a tracker for it though mapbox/mapbox-navigation-android-examples#128 |
Description
In order to create stable apis for Android Auto and DropInUi, the
MapboxNavigationApp
framework needs to also be stable. We have also reached a point where it would cause many issues downstream if changes are not backwards compatible.This also deprecates
MapboxNavigationProvider
. AdoptingMapboxNavigationApp
will not break any references toMapboxNavigationProvider
because it is used by MapboxNavigationOwner.cc: @mapbox/navigation-android